How about checking if the size is different from the old recorded size? I believe the data engine will prune changes where a value is written to a cell that already contains that value, but seems easy to cut it out at the source here.

Can you move this test over using DIFF? I see that you've changed the content of this file, and I don't have tools to show me the what was changed.

I can tag which bits are the same? I've done a restructuring on this file to make testing with external attachments easier, so I'm not sure a diff would show many useful things.

I just did a rebase that should make this easier.

This commit has all the DocumentUsage changes in:
c90b777

New commit hash, had to do a rebase to sort a merge issue:
aacb5ee

Can you maybe add another file for testing only external attachments and leave this one untouched?

Most of these tests fail if untouched 😅

I can isolate and duplicate the attachments tests if you want, and ensure they pass under both external and internal.

But I'm also not seeing the harm in having a pure-core version of this file? Happy to discuss further.

After discussion:

• Several of the non-attachments related tests will be moved to a new PR
• This file will be renamed

I think you end up with old credentials, not bad. You can be logged into grist using multiple accounts at the same time, this is a feature :).

I've done a slight rephrasing.

The original session ends up with old credentials, you're right! But any new session ends up with bad credentials - specifically the bearer token ends up as api_key_for_chimpy, which just gives us the wrong responses from the API 😅

Hide whitespace changes for this diff - it'll make it much smaller. A lot of it was just indentation changes.

Haven't found usage of the second argument, and this whole function looks very similar to the one above.

What do you think about:
Changing line 3611 to:
process.env[key] = typeof vars[key] === 'function' ? await vars[key]() : vars[key];

Changing this function to

export function enableExternalAttachments(transferDelay?: string) {
  withEnvironmentSnapshot({
    GRIST_EXTERNAL_ATTACHMENTS_MODE: 'test',
    GRIST_TEST_TRANSFER_DELAY: transferDelay ?? "",
    GRIST_TEST_ATTACHMENTS_DIR: async () => mkdtemp(path.join(await createTmpDir(), 'attachments'))
  });
}

And just reading process.env.GRIST_TEST_ATTACHMENTS_DIR whenever we need this information in tests?

The main difference between this function and withEnvironmentSnapshot is it only touches specific variables, rather than the whole environment.

The reason is there's a lower chance of accidental issues / conflicts this way, such as when multiple environment snapshots are in use!

That's also why it has that second parameter - to disable the environment restore if it's causing problems.

Let me see if I can deduplicate this a bit though, if that's the main issue here.

I've integrated all the behaviour I wanted into EnvironmentSnapshot and withEnvironmentSnapshot.

You can now pass them specific variables you want to save/set, and it'll save / restore only those variables, minimising the chance of conflicting changes.

The key reason I want this here specifically is because with gu.enableExternalAttachments, it's not apparent that the environment is being modified - increasing the risk of an accidental problem.

Ok, so sorry for the comment above, I'm lost here a little bit with the logic you created. For me this was a simple stack, there was only one env at a time, it was a snapshot not a patch, there are no multiple environment in use (I don't know even what that means for you).

I don't get it, why do you need to touch only specific variables, and how do you create multiple process at the same time? I tested my proposition before, and it worked fine.

Can you revert those changes to the one you had before? Now it is very hard to understand what is going on here.

As discussed, I've moved the helper out to its own file. :)

Is it worth checking if rowIdsToUpdate is empty and returning earlier (without all external calls)?

Saas linter will mark this. We have a rule: objects without spaces, scopes with spaces.

Thanks for the warning, might be worth bringing that rule over to core!

Please do, I already tried and failed, but don't remember the specifics.

This is just a refactor, am I right? I dont see any actual code change.

Yeah - the uses for server and testUtils were replaced by a call to gu.enableExternalAttachments

I think it is better to inline this method. It is not used anywhere else.

Keep this one as is - it's not used here right now, because I moved the extra tests to another branch.

When I open a PR for that, this will be used in multiple places again :)

Hmmm, we usually avoid doing this that way. If it is not used right now in multiple plays, lets introduced this abstraction later on.

Alright, will pull this out.

What do you think about wrapping it with try catch ? Asking because, this will throw if ids are not valid, it is very unlikely but still possible if the docData we have in memory is not exactly the same as the one in data engine, everything here is async, so maybe it can somehow interfere with other modifications.

Ideally this would all be done in "transaction like" scope, but I don't know if it is important to worry about that here. We have some mechanism for locking the doc modifications, but maybe this is overkill here. But there is other mechanism available - the upsert action BulkAddOrUpdateRecord, where you can update those values based on the fileIdentity, not the ids.

This is a difficult question. The options seem to be:

• Don't catch, fail to import the attachments
• Catch, import the attachments with an incorrect file size

If this is very rare, I think I'd prefer to let it fail and force the user to re-import, than import attachments with an invalid file size.

I don't think upsert is an option unfortunately, as it's not correct to add a new record if it doesn't exist. Best case, we have an extra attachment being tracked that doesn't exist. Worst case, it breaks other code because it's not a fully valid record that's added (unless we try to reinsert the whole record...).

My inclination is to leave it, add a comment, see if it actually comes up in production.

How do other parts of the codebase handle the in-memory data and data engine being out of sync? Just locking?

The minimum was to wrap this in try catch, and rethrow something more meaningful, explaining what has happen.

I don't know for sure, but we have some data corruptions problems we don't know how to fix or don't know the reason. So here, we know that the ids can go wrong, so we can anticipate it and explain in more details.
Since we don't do any performance or real load tests, waiting for this to happen on prod is a wrong idea, we don't know anything about how this would work on prod.

I just checked the docstring for the BulkAddOrUpdateRecord, there is a on option parameter that disables adding if it can't find records to update, so I think it actually does look as a good option, wdyt? Here is a docstring:

   `options` is a dictionary with optional settings to choose other behaviours:
    - Set "on_many" to "all" or "none" to change which records are updated when several match.
    - Set "update" or "add" to False to disable updating or adding records respectively,
      i.e. if you only want to add records that don't already exist
        or if you only want to update records that do already exist.
    - Set "allow_empty_require" to True to allow `require` to be an empty dictionary,
      which would mean that every record in the table is matched.
      Otherwise this will raise an error to prevent mistakes like updating an entire column.

I tried this, but apparently that method doesn't support metatables ☹️

Instead, I've wrapped it in a more specific error, and added some logging around that error too. How does that look?

I think this is not needed now, right? I think we fixed this issue.

The problem was avoided in a different way, by not restarting the server except at the start of the test file.

I can remove this as a result, but at the same time, it would be nice to document this behaviour somewhere. But not sure where.

But I meant, that we actually fixed this issue in other PR, but I can't find it now.

My memory was that this was intended behaviour and we weren't changing it, but my memory might be wrong! :)

The convention is to have the same name as the file.

When you change the document through the API, you are not waiting for those change to propagate back to the browser. It works 99% of time, but introduce flakines.

The second waitForServer is not needed I think, is it?

Your helper for adding attachments column is doing the same thing that this call:
await gu.addColumn('Attachments', 'Attachment');

Thanks for the advice, will put in these changes.
Not entirely sure if it's needed or not. I'll double check.

Made these changes, you're 100% right, that second waitForServer isn't needed - gu.sendActions waits :)

This helper is not needed, there is a type parameter in the addColumn, which does the same thing as you.

Great to know, removed it :)